Skip to content

GithubのSSO機能#67

Merged
mochi-yu merged 9 commits intomainfrom
feature/github-login
Dec 15, 2023
Merged

GithubのSSO機能#67
mochi-yu merged 9 commits intomainfrom
feature/github-login

Conversation

@cocoide
Copy link
Copy Markdown
Owner

@cocoide cocoide commented Dec 10, 2023

概要

デバイスフロー(参照)で、サーバーにアクセスせずにアクセストークンを発行して、configに保存するコマンド
(commitify login)を実装

補足

  • 認証の部分はポーリング処理をしているのだが、その処理を止めないためにsleepしており、そこを完了したらsleepを解除する仕様に後ほど変えようと考えてます。(時間がないので一旦PR機能を先に作ります)
  • protoファイルを他のrepoのを参照していてよくなかったので、srcに定義ファイル、genに生成ファイルを置く形式で参照元を修正

@cocoide cocoide requested a review from mochi-yu December 10, 2023 14:02
@mochi-yu
Copy link
Copy Markdown
Collaborator

mainに直マージでいいの?

}

func NewLoginCmdUsecase(http *gateway.HttpClient) *LoginCmdUsecase {
http.WithBaseURL("https://github.com/login")
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

これ、フィールドに直接代入するのではなくて、WithBaseURL()みたいなメソッドを経由しているのはなぜ?
もともとEndpointのフィールドとかはPublicで直接代入できるよね?

Copy link
Copy Markdown
Owner Author

@cocoide cocoide Dec 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

このコードは使用方法が少し不適切なんだけど
メソッドチェーンを利用してるのは、WithBaseURL().WithBody().Excute("POST")みたいに
1行で書く量を減らせるから


const (
GithubClientID = "b27d87c28752d2363922"
GithubScope = "repo"
Copy link
Copy Markdown
Collaborator

@mochi-yu mochi-yu Dec 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

リポジトリの情報までスコープに入れる必要あるかな?公開されているパブリック情報だけでも、UIDとかは持ってこれる気もするけど
https://docs.github.com/ja/apps/oauth-apps/building-oauth-apps/scopes-for-oauth-apps#available-scopes

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

たしかに、scopeはもう少し限定してもいいかもね

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

限定するというか、何も書かなければ最低限の情報しか取得しなくなるんじゃないかな。

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

うんその認識だよ
(例)repo:〇〇で
pull requestを出すのに必要最低限のscopeにするってことだよね

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prが可能かどうかのscopeがパッとせんから一旦これはrepoでよろしく🙏

@cocoide
Copy link
Copy Markdown
Owner Author

cocoide commented Dec 13, 2023

mainに直マージでいいの?

相談してなくてすまん
リリースがtagでやるのと、時間がないから直接mainでも問題ないと思った!

Comment thread internal/gateway/http_client.go Outdated
Comment on lines +11 to +15
Client *http.Client
Endpoint string
Headers map[string]string
Params map[string]interface{}
Body io.Reader
Copy link
Copy Markdown
Collaborator

@mochi-yu mochi-yu Dec 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

メソッドチェーンでやるならここら辺プライベートでも良いかなと思ったけど、まあ時間ないならどちらでも

Suggested change
Client *http.Client
Endpoint string
Headers map[string]string
Params map[string]interface{}
Body io.Reader
Client *http.Client
endpoint string
headers map[string]string
params map[string]interface{}
body io.Reader

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

そうやな、すまん修正しておく!

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2ddf00a
done here

@cocoide
Copy link
Copy Markdown
Owner Author

cocoide commented Dec 15, 2023

@mochi-yu
非同期処理気になってた点も追加で修正した!

@mochi-yu mochi-yu merged commit 5455880 into main Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants